ERC7579: prevent installing/uninstalling without proper initData #5974

Amxx · 2025-10-06T15:13:22Z

Followup to #5961
Replaces #5971

PR Checklist

Tests
Documentation
Changeset entry (run npx changeset add)

changeset-bot · 2025-10-06T15:13:26Z

⚠️ No Changeset found

Latest commit: f5a1bb7

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

coderabbitai · 2025-10-06T15:16:44Z

Walkthrough

In certora/specs/Account.spec, the fallbackModule rule now requires initData.length >= 4 before checking getFallbackHandler(getDataSelector(initData)) == module.
In contracts/account/extensions/draft-AccountERC7579.sol, two new public errors were added: ERC7579CannotDecodeFallbackData() and ERC7579InvalidModuleSignature(). _extractSignatureValidator now validates signature length and extracts the full 20-byte module; _decodeFallbackData enforces a minimum data length.
In tests, new cases assert that installing/uninstalling a fallback module with initData shorter than 4 bytes reverts with ERC7579CannotDecodeFallbackData.

Suggested labels

ignore-changeset

Pre-merge checks and finishing touches

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.
Description Check	❓ Inconclusive	The provided description only includes references to previous PRs and a checklist without summarizing the actual changes to ERC7579 validation or the new error handling and tests, making it too generic to convey meaningful context for reviewers. It fails to contextualize the code changes or explain the rationale behind the added validation logic. While it is related in a meta sense, it remains too vague and offers no substantive summary of the updates. A clearer summary would help reviewers understand the purpose and scope of the changes without inspecting individual files.	Add a concise summary of the specific changes introduced in this PR, such as the updated validation guards in AccountERC7579, the new errors, and the corresponding test cases.

✅ Passed checks (1 passed)

Check name	Status	Explanation
Title Check	✅ Passed	The title clearly summarizes the primary change by indicating that ERC7579 will now prevent installing or uninstalling a module without proper initData, directly reflecting the added input validation in AccountERC7579. It is concise and specific, avoiding file lists or generic terminology. This phrasing gives teammates an immediate understanding of the main change when scanning the PR history.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch feature/prevent-fallback-module-install-uninstall-without-selector

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 765b288 and 6c8aaca.

📒 Files selected for processing (3)

certora/specs/Account.spec (1 hunks)
contracts/account/extensions/draft-AccountERC7579.sol (3 hunks)
test/account/extensions/AccountERC7579.behavior.js (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

test/account/extensions/AccountERC7579.behavior.js (1)

test/helpers/erc7579.js (1)

MODULE_TYPE_FALLBACK (5-5)

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)

GitHub Check: tests-foundry
GitHub Check: tests-upgradeable
GitHub Check: slither
GitHub Check: tests
GitHub Check: coverage
GitHub Check: halmos
GitHub Check: verify

🔇 Additional comments (6)

certora/specs/Account.spec (1)

208-208: LGTM! Spec now aligns with implementation.

The guard correctly ensures the fallback module relationship is only asserted when initData contains a valid selector (≥4 bytes), matching the new validation in _decodeFallbackData.

contracts/account/extensions/draft-AccountERC7579.sol (3)

71-75: LGTM! Clear error declarations.

The new errors provide clear feedback when data is too short to decode. Documentation comments accurately describe when each error is thrown.

393-394: LGTM! Defensive validation.

The require adds input validation directly in the internal function, which is good practice even though the caller at line 192 already checks signature.length >= 20. This protects against future refactoring or additional call sites.

409-409: LGTM! Critical validation fix.

This check prevents silent zero-padding when data.length < 4. Without it, bytes4(data) would return a selector with trailing zeros (e.g., 0x12000000 for input 0x12), allowing fallback modules to be installed/uninstalled with incorrect selectors.

test/account/extensions/AccountERC7579.behavior.js (2)

170-179: LGTM! Good edge case coverage.

The tests verify that both completely empty (0x) and too-short (0x123456 = 3 bytes) initData properly revert with the new error. This ensures the validation catches all invalid inputs.

254-263: LGTM! Symmetric test coverage.

Good coverage of the uninstallation path with the same edge cases as installation. This ensures _decodeFallbackData is called correctly from both _installModule and _uninstallModule.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

ernestognw · 2025-10-06T15:16:55Z

Not sure if a changeset is required. I would mention the addition of the custom errors but that's probably the only thing to note. People don't rely on errors for critical functionality anyway

Amxx · 2025-10-06T15:17:45Z

I think it could be nice to have a changeset. This is technically breaking

ernestognw · 2025-10-06T20:28:10Z

contracts/account/extensions/draft-AccountERC7579.sol

    ) internal pure virtual returns (address module, bytes calldata innerSignature) {
-        return (address(bytes20(signature[0:20])), signature[20:]);
+        require(signature.length > 19, ERC7579InvalidModuleSignature());
+        return (address(bytes20(signature)), signature[20:]);


I just realized that isValidSignature is already avoiding that this function is called with a signature that's less than 20 bytes long:

function isValidSignature(bytes32 hash, bytes calldata signature) public view virtual returns (bytes4) { if (signature.length >= 20) { (address module, bytes calldata innerSignature) = _extractSignatureValidator(signature); ...

I would suggest reverting this change and just noting that the function does expect a signature of at least 20 bytes. Otherwise this will add an extra (and arguably unnecessary) check.

ernestognw

LGTM if you agree with keeping _extractSignatureValidator as it is right now

Amxx · 2025-10-07T07:21:10Z

contracts/account/extensions/draft-AccountERC7579.sol

     *
     * This is not standardized in ERC-7579 (or in any follow-up ERC). Some accounts may want to override these internal functions.
+     *
+     * NOTE: This function expects the signature to be at least 20 bytes long. Panics with {Panic-ARRAY_OUT_OF_BOUNDS} (0x32) otherwise.


@ernestognw Any reason to prefer a Panic over a custom error ?

ERC7579: prevent installing/uninstalling without proper initData

6c8aaca

Amxx added this to the 5.5-final milestone Oct 6, 2025

Amxx requested review from arr00, james-toussaint and ernestognw October 6, 2025 15:13

Amxx requested a review from a team as a code owner October 6, 2025 15:13

Amxx added bug formal-verification Enable FV run in a PR. Account Abstraction labels Oct 6, 2025

ernestognw previously approved these changes Oct 6, 2025

View reviewed changes

Revert _extractSignatureValidator and add note

d5f9434

ernestognw mentioned this pull request Oct 6, 2025

Fix invalid length check in SignatureChecker #5973

Merged

3 tasks

ernestognw reviewed Oct 6, 2025

View reviewed changes

ernestognw dismissed their stale review via d5f9434 October 6, 2025 20:38

Remove ERC7579CannotDecodeFallbackData

f5a1bb7

ernestognw approved these changes Oct 6, 2025

View reviewed changes

Amxx commented Oct 7, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

ERC7579: prevent installing/uninstalling without proper initData #5974

ERC7579: prevent installing/uninstalling without proper initData #5974

Amxx commented Oct 6, 2025

Uh oh!

changeset-bot bot commented Oct 6, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Oct 6, 2025

Uh oh!

ernestognw commented Oct 6, 2025

Uh oh!

Amxx commented Oct 6, 2025

Uh oh!

ernestognw Oct 6, 2025

Uh oh!

ernestognw left a comment

Uh oh!

Amxx Oct 7, 2025

Uh oh!

Uh oh!

ERC7579: prevent installing/uninstalling without proper initData #5974

Are you sure you want to change the base?

ERC7579: prevent installing/uninstalling without proper initData #5974

Conversation

Amxx commented Oct 6, 2025

PR Checklist

Uh oh!

changeset-bot bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

⚠️ No Changeset found

Uh oh!

coderabbitai bot commented Oct 6, 2025

Walkthrough

Suggested labels

Pre-merge checks and finishing touches

Uh oh!

ernestognw commented Oct 6, 2025

Uh oh!

Amxx commented Oct 6, 2025

Uh oh!

ernestognw Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

ernestognw left a comment

Choose a reason for hiding this comment

Uh oh!

Amxx Oct 7, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

changeset-bot bot commented Oct 6, 2025 •

edited

Loading